[NRBF] More bug fixes #107682
Merged
[NRBF] More bug fixes #107682
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…pected by switch statements and if/else blocks
…lue). This is something I agreed on with Jeremy Kuhne a while ago, but missed it.
…the value of the Id in explicit way
…fo (Length property is longer present in this type)
|
@MihuBot fuzz NrbfDecoder |
|
3 tasks
… a place where it's not valid
adamsitnik
commented
Sep 11, 2024
| @@ -13,7 +13,7 @@ namespace System.Formats.Nrbf; | |||
| /// <remarks> | |||
| /// ArrayInfo structures are described in <see href="https://learn.microsoft.com/openspecs/windows_protocols/ms-nrbf/8fac763f-e46d-43a1-b360-80eb83d2c5fb">[MS-NRBF] 2.4.2.1</see>. | |||
| /// </remarks> | |||
| [DebuggerDisplay("Length={Length}, {ArrayType}, rank={Rank}")] | |||
There was a problem hiding this comment.
This type no longer provides Length property.
adamsitnik
commented
Sep 11, 2024
src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs
Show resolved
Hide resolved
adamsitnik
commented
Sep 11, 2024
|
|
||
| values.Add(value); | ||
| } | ||
| #else |
There was a problem hiding this comment.
this perf optimization was wrong because it was not checking if given string input is a valid utf8 string
|
@MihuBot fuzz NrbfDecoder |
adamsitnik
commented
Sep 11, 2024
src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/BinaryArrayRecord.cs
Show resolved
Hide resolved
adamsitnik
commented
Sep 11, 2024
| if (read == 0) | ||
| throw new EndOfStreamException(); | ||
| offset += read; | ||
| return false; |
There was a problem hiding this comment.
the goal of this change is to simply return false rather than throw EOSE
There was a problem hiding this comment.
Do we need to set the stream position back to the beginning before we return here?
There was a problem hiding this comment.
Do we need to set the stream position back to the beginning before we return here?
Great catch!
adamsitnik
commented
Sep 11, 2024
adamsitnik
commented
Sep 11, 2024
| : new LinkedList<object>(); | ||
| #endif | ||
|
|
||
| // ArrayInfo.GetSZArrayLength ensures to return a value <= Array.MaxLength |
There was a problem hiding this comment.
Since now we don't allow for any MD array with number of elements > Array.MaxLength, we don't need to use LinkedList anymore
adamsitnik
commented
Sep 11, 2024
| @@ -176,7 +170,11 @@ internal static RectangularArrayRecord Create(BinaryReader reader, ArrayInfo arr | |||
| PrimitiveType.Int64 => sizeof(long), | |||
| PrimitiveType.UInt64 => sizeof(ulong), | |||
| PrimitiveType.Double => sizeof(double), | |||
| _ => -1 | |||
| PrimitiveType.TimeSpan => sizeof(ulong), | |||
| PrimitiveType.DateTime => sizeof(ulong), | |||
There was a problem hiding this comment.
this is an improvement that I got into when I was making all the switch statements throw in the default case, we were not handling TimeSpan and DateTime properly before
adamsitnik
commented
Sep 11, 2024
src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/RectangularArrayRecord.cs
Outdated
Show resolved
Hide resolved
adamsitnik
commented
Sep 11, 2024
| @@ -15,6 +16,7 @@ namespace System.Formats.Nrbf; | |||
| /// <summary> | |||
| /// The ID of <see cref="SerializationRecord" />. | |||
| /// </summary> | |||
| [DebuggerDisplay("{_id}")] | |||
There was a problem hiding this comment.
It's just user experience improvement
|
… complete, but increases the amount of code that needs to be reviewed
…vided in a place where it's not valid
|
@MihuBot fuzz NrbfDecoder |
|
@MihuBot fuzz NrbfDecoder |
|
|
JeremyKuhne
approved these changes
Sep 11, 2024
bartonjs
reviewed
Sep 11, 2024
| break; | ||
| default: | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
FWIW, InvalidOperationException is for when a method is invoked on an object and the object has state that doesn't support it. For example, calling Stream.Read when Stream.CanRead says false. The object is not in a state where you can call the method.
Many of yours are probably more likely InvalidDataException, where the default message is "Found invalid data while decoding."
There was a problem hiding this comment.
Immo corrected me. Stream.Read when Stream.CanRead is false is NotSupportedException (you should have checked the property on the stream). InvalidOperationException is for controllable state that was controlled wrong, like an object where you have to set 3 properties before calling "DoStuff", and you called DoStuff without setting all 3.
But this is still most likely InvalidDataException, because the operation made sense to do, just the data that it read back was not internally consistent.
There was a problem hiding this comment.
What I wanted to achieve is:
- ensure all methods that parse enums throw SerializationException when they encounter invalid value (out of range, not in the spec)
- assume that the above is not true and always handle all enum values in switch statements in explicit way, throw in the default case (so if there is a bug in parsing logic, we don't get undefined behavior but just an exception)
My initial idea was to use UnreachableException, but it's not part of NS2.0. I decided to use InvalidOperationException and I am open to changing it.
It's being tested by the Fuzzer which does not catch InvalidOperationException and treats it as a bug:
runtime/src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/NrbfDecoderFuzzer.cs
Lines 100 to 106 in 4930e1b
(as shown in the fuzzer comments in this PR that has discovered places where it was missing)
… is always illegal to parse it and every switch needed to take this into account. Now we reject those values at parse time and don't need to handle them later. - In case of BinaryArray (NRBF 2.4.3.1): "If the BinaryTypeEnum value is Primitive, the PrimitiveTypeEnumeration value in AdditionalTypeInfo MUST NOT be Null (17) or String (18)." - In case of MemberPrimitiveTyped (NRBF 2.5.1): "PrimitiveTypeEnum (1 byte): A PrimitiveTypeEnumeration value that specifies the Primitive Type of data that is being transmitted. This field MUST NOT contain a value of 17 (Null) or 18 (String)." - In case of ArraySinglePrimitive (NRBF 2.4.3.3): "A PrimitiveTypeEnumeration value that identifies the Primitive Type of the items of the Array. The value MUST NOT be 17 (Null) or 18 (String)." - In case of MemberTypeInfo (NRBF 2.3.1.2): "When the BinaryTypeEnum value is Primitive, the PrimitiveTypeEnumeration value in AdditionalInfo MUST NOT be Null (17) or String (18)."
|
@MihuBot fuzz NrbfDecoder |
adamsitnik
added a commit
to adamsitnik/runtime
that referenced
this pull request
Sep 13, 2024
- Don't use `Debug.Fail` not followed by an exception (it may cause problems for apps deployed in Debug) - avoid Int32 overflow - throw for unexpected enum values just in case parsing has not rejected them - validate the number of chars read by BinaryReader.ReadChars - pass serialization record id to ex message - return false rather than throw EndOfStreamException when provided Stream has not enough data - don't restore the position in finally - limit max SZ and MD array length to Array.MaxLength, stop using LinkedList<T> as List<T> will be able to hold all elements now - remove internal enum values that were always illegal, but needed to be handled everywhere - Fix DebuggerDisplay
4 tasks
jtschuster
pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
- Don't use `Debug.Fail` not followed by an exception (it may cause problems for apps deployed in Debug) - avoid Int32 overflow - throw for unexpected enum values just in case parsing has not rejected them - validate the number of chars read by BinaryReader.ReadChars - pass serialization record id to ex message - return false rather than throw EndOfStreamException when provided Stream has not enough data - don't restore the position in finally - limit max SZ and MD array length to Array.MaxLength, stop using LinkedList<T> as List<T> will be able to hold all elements now - remove internal enum values that were always illegal, but needed to be handled everywhere - Fix DebuggerDisplay
carlossanlop
pushed a commit
that referenced
this pull request
Sep 17, 2024
* [NRBF] Don't use Unsafe.As when decoding DateTime(s) (#105749) * Add NrbfDecoder Fuzzer (#107385) * [NRBF] Fix bugs discovered by the fuzzer (#107368) * bug #1: don't allow for values out of the SerializationRecordType enum range * bug #2: throw SerializationException rather than KeyNotFoundException when the referenced record is missing or it points to a record of different type * bug #3: throw SerializationException rather than FormatException when it's being thrown by BinaryReader (or sth else that we use) * bug #4: document the fact that IOException can be thrown * bug #5: throw SerializationException rather than OverflowException when parsing the decimal fails * bug #6: 0 and 17 are illegal values for PrimitiveType enum * bug #7: throw SerializationException when a surrogate character is read (so far an ArgumentException was thrown) # Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/NrbfDecoder.cs * [NRBF] throw SerializationException when a surrogate character is read (#107532) (so far an ArgumentException was thrown) * [NRBF] Fuzzing non-seekable stream input (#107605) * [NRBF] More bug fixes (#107682) - Don't use `Debug.Fail` not followed by an exception (it may cause problems for apps deployed in Debug) - avoid Int32 overflow - throw for unexpected enum values just in case parsing has not rejected them - validate the number of chars read by BinaryReader.ReadChars - pass serialization record id to ex message - return false rather than throw EndOfStreamException when provided Stream has not enough data - don't restore the position in finally - limit max SZ and MD array length to Array.MaxLength, stop using LinkedList<T> as List<T> will be able to hold all elements now - remove internal enum values that were always illegal, but needed to be handled everywhere - Fix DebuggerDisplay * [NRBF] Comments and bug fixes from internal code review (#107735) * copy comments and asserts from Levis internal code review * apply Levis suggestion: don't store Array.MaxLength as a const, as it may change in the future * add missing and fix some of the existing comments * first bug fix: SerializationRecord.TypeNameMatches should throw ArgumentNullException for null Type argument * second bug fix: SerializationRecord.TypeNameMatches should know the difference between SZArray and single-dimension, non-zero offset arrays (example: int[] and int[*]) * third bug fix: don't cast bytes to booleans * fourth bug fix: don't cast bytes to DateTimes * add one test case that I've forgot in previous PR # Conflicts: # src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecord.cs * [NRBF] Address issues discovered by Threat Model (#106629) * introduce ArrayRecord.FlattenedLength * do not include invalid Type or Assembly names in the exception messages, as it's most likely corrupted/tampered/malicious data and could be used as a vector of attack. * It is possible to have binary array records have an element type of array without being marked as jagged --------- Co-authored-by: Buyaa Namnan <bunamnan@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes issues discovered offline by @GrabYourPitchforks